Add RETURNDATACOPY and RETURNDATASIZE to assembly (and LLL) #2275

pirapira · 2017-05-16T16:55:07Z

This PR adds RETURNDATACOPY and RETURNDATASIZE in LLL so that Metropolis tests in etheruem/tests can be developed more easily. In passing, I added minimum documentation and tests about inline assembly.

axic · 2017-05-17T08:05:41Z

libevmasm/Instruction.h

@@ -49,6 +49,8 @@ enum class Instruction: uint8_t
 	MULMOD,				///< unsigned modular multiplication
 	EXP,				///< exponential operation
 	SIGNEXTEND,			///< extend length of signed integer
+	RETURNDATASIZE = 0xd,	///< get size of the last return data


We've agreed to a different opcode on the last call.

Thank you for the info. I'll update my yellow paper PR, the test PR and this one.

I moved them to 0x3d and 0x3e.

axic · 2017-05-17T08:05:56Z

docs/assembly.rst

@@ -182,6 +182,10 @@ In the grammar, opcodes are represented as pre-defined identifiers.
 +-------------------------+------+-----------------------------------------------------------------+
 | signextend(i, x)        |      | sign extend from (i*8+7)th bit counting from least significant  |
 +-------------------------+------+-----------------------------------------------------------------+
+| returndatasize          |      | size of the last return data                                    |


Please move this to the right location after the opcode is updated.

axic · 2017-05-17T08:06:55Z

test/liblll/Parser.cpp

@@ -174,6 +174,16 @@ BOOST_AUTO_TEST_CASE(list)
 	BOOST_CHECK(!successParse("()"));
 }

+BOOST_AUTO_TEST_CASE(returndatacopy)
+{
+	BOOST_CHECK(successParse("( returndatacopy 0 1 2 )"));


I do not think there's a need for parser test. The parser doesn't care.

I agree. The test succeeded even before the change. So this does not add any value.

So I removed this test and the next similar one.

axic · 2017-05-17T08:07:34Z

test/liblll/EndToEndTest.cpp

+BOOST_AUTO_TEST_CASE(returndatasize_can_be_compiled)
+{
+	char const* sourceCode = R"(
+		(returnlll


I'm not sure this is needed (though probably cannot hurt), because LLL just exposes every single opcode. Potentially we should test for every single one? :)

This was the motivation of the whole PR.

But we do not test for 99% of the opcodes.

Let me see if there is any other opcode tested this way.

OK, I can guess from the other tests that opcodes are visible. I see sstore, codecopy, mstore8, div, exp and so on. So I don't need this test.

axic

Please update the opcode.

axic · 2017-05-17T08:09:39Z

It is also missing the changelog entry and the optimiser changes (GasMeter, SemanticInformation).

pirapira · 2017-05-17T11:41:16Z

I think I updated everything.

axic · 2017-05-19T18:59:15Z

libevmasm/Instruction.cpp

@@ -203,6 +205,8 @@ static const std::map<Instruction, InstructionInfo> c_instructionInfo =
 	{ Instruction::GASPRICE,	{ "GASPRICE",		0, 0, 1, false, Tier::Base } },
 	{ Instruction::EXTCODESIZE,	{ "EXTCODESIZE",	0, 1, 1, false, Tier::ExtCode } },
 	{ Instruction::EXTCODECOPY,	{ "EXTCODECOPY",	0, 4, 0, true, Tier::ExtCode } },
+	{ Instruction::RETURNDATASIZE, {"RETURNDATASIZE",	0, 0, 1, false, Tier::Base } },


Indentation is wrong here

axic · 2017-05-19T19:00:04Z

scripts/tests.sh

@@ -71,9 +71,9 @@ sleep 2
 # And then run the Solidity unit-tests (once without optimization, once with),
 # pointing to that IPC endpoint.
 echo "--> Running tests without optimizer..."
-  "$REPO_ROOT"/build/test/soltest --show-progress -- --ipcpath /tmp/test/geth.ipc && \
+  "$REPO_ROOT"/build/test/soltest --show-progress -t '*/returndata*' -- --ipcpath /tmp/test/geth.ipc && \


Shouldn't filter for returndata here :)

Oh, sorry. This change is garbage.

Reverted these changes.

axic · 2017-05-24T14:13:11Z

docs/assembly.rst

@@ -232,6 +232,10 @@ In the grammar, opcodes are represented as pre-defined identifiers.
 +-------------------------+------+-----------------------------------------------------------------+
 | extcodecopy(a, t, f, s) | `-`  | like codecopy(t, f, s) but take code at address a               |
 +-------------------------+------+-----------------------------------------------------------------+
+| returndatasize          |      | size of the last return data                                    |
+-------------------------+------+-----------------------------------------------------------------+
+| returndatacopy(t, f, s) | `*`  | copy s bytes from returndata at position f to mem at position t |


Either we should call it returndataor return data in both places.

You're right.

axic · 2017-05-24T15:17:55Z

I think we should have warnings in place if using these Metropolis instructions. This applies to CREATE and STATICCALL too.

axic · 2017-05-24T15:27:03Z

Added warnings to the analyzer, will push it later the evening.

chriseth · 2017-05-24T15:27:34Z

Please add a warning if in the scope of an inline assembly block there is a solidity identifier called returndatasize.

axic · 2017-05-24T15:29:52Z

test/libsolidity/InlineAssembly.cpp

@@ -429,6 +429,16 @@ BOOST_AUTO_TEST_CASE(revert)
 	BOOST_CHECK(successAssemble("{ revert(0, 0) }"));
 }

+BOOST_AUTO_TEST_CASE(returndatasize)
+{
+	BOOST_CHECK(successAssemble("{ let r := returndatasize }"));


Also potentially this should be in "functional notation" to be in line with returndatacopy / revert.

There is an exception that for an opcode x, if x() is valid, you can omit the parentheses. We could make that more strict (it is more strict in julia already).

Both are valid, it is only about consistency:

do not test for these

test with one notation only (but use the same for these "opcode tests")

test with both notations (this has its own merits, given the code paths in the parser/analyzer are different)

I think we should test random styles on random instructions to catch different kinds of errors. I don't think we get coverage efficiently if we tested the same set of styles for all instructions.

If we want to catch as many bugs as possible we should test for both notations.

But, if no other instruction is tested for "both styles", maybe now is a good time to do that for at least one instruction.

It is the opposite, practically no "functional instructions" are tested in the assembler section.

add is already tested for both styles in functional, but it has two arguments: not zero or three.

You have to look at the SolidityInlineAssembler/Analysis suite. The others don't assemble.

Right, and at this point, it's easier to add both styles.

axic · 2017-05-24T21:08:49Z

libsolidity/inlineasm/AsmAnalysis.cpp

+	case solidity::Instruction::RETURNDATACOPY:
+		m_errors.push_back(make_shared<Error>(
+			Error::Type::Warning,
+			"The RETURNDATASIZE/RETURNDATACOPY instructions are only available after "


Any better suggestion for wording?

@pirapira @chriseth can you please review this?

Doesn't STATICCALL belong here?

And CREATE2 (and REVERT with a different message), but those are not merged yet.

STATICCALL has already been merged.

It is pending as #2192?

I think I was seeing cpp-ethereum. Sorry.

@chriseth @pirapira this is the main question in this PR though: what wording do we go with 😉

This looks perfect to me. It should be a warning, it should mention Metropolis, and it should tell this and that instructions will become available with Metropolis.

@chriseth ?

chriseth · 2017-06-12T15:11:11Z

libsolidity/analysis/NameAndTypeResolver.cpp

+	{
+		string instructionName{instruction.first}; // needs to copy because making it lower case.
+		boost::algorithm::to_lower(instructionName);
+		auto declarations = nameFromCurrentScope(instructionName);


Did you test whether this always reports the global sha3?

Right, that might be happening; there might be a warning about sha3 on every possible source code with inline assembly. I should add a test about that.

axic · 2017-06-12T22:35:42Z

libsolidity/analysis/NameAndTypeResolver.cpp

+			solAssert(!!declaration, "");
+			m_errorReporter.warning(
+				declaration->location(),
+				"Variable is shadowed in an inline assembly by an insturction of the same name"


"in an" -> "in" and "instruction" -> "instruction"?

#2275 (comment)

chriseth · 2017-06-13T14:36:33Z

Please merge when tests are green.

axic · 2017-06-13T14:43:37Z

Can you please squash some commits? The "fix typo" commit only fixed the code and there will be another one fixing the tests 😉

Could be merged into the original commit.

#2275 (comment)

pirapira · 2017-06-13T14:58:18Z

@axic squashed a bit.

pirapira added the in progress label May 16, 2017

axic changed the title ~~Add RETURNDATACOPY and RETURNDATASIZE in LLL~~ Add RETURNDATACOPY and RETURNDATASIZE to assembly (and LLL) May 17, 2017

axic reviewed May 17, 2017

View reviewed changes

axic requested changes May 17, 2017

View reviewed changes

pirapira added the needs review label May 17, 2017

pirapira mentioned this pull request May 17, 2017

EIP211: RETURNDATA ethereum/aleth#4062

Merged

axic reviewed May 19, 2017

View reviewed changes

pirapira force-pushed the returndata_lll branch from 6d931ae to b16a4b5 Compare May 22, 2017 08:55

chriseth approved these changes May 24, 2017

View reviewed changes

axic force-pushed the returndata_lll branch from b16a4b5 to 55aed7e Compare May 24, 2017 12:53

axic approved these changes May 24, 2017

View reviewed changes

axic force-pushed the returndata_lll branch 5 times, most recently from 9e414af to 1bfae8d Compare May 24, 2017 13:04

axic reviewed May 24, 2017

View reviewed changes

axic force-pushed the returndata_lll branch from 3fb3492 to 31e727c Compare May 24, 2017 21:04

axic reviewed May 24, 2017

View reviewed changes

axic force-pushed the returndata_lll branch from 31e727c to e88b964 Compare May 29, 2017 12:28

chriseth reviewed Jun 12, 2017

View reviewed changes

axic reviewed Jun 12, 2017

View reviewed changes

pirapira force-pushed the returndata_lll branch from 6231916 to b68347e Compare June 13, 2017 14:31

pirapira added a commit that referenced this pull request Jun 13, 2017

Add a failing test as suggested in

498acbc

#2275 (comment)

pirapira added a commit that referenced this pull request Jun 13, 2017

Fix typo

b68347e

#2275 (comment)

chriseth approved these changes Jun 13, 2017

View reviewed changes

axic closed this Jun 13, 2017

axic reopened this Jun 13, 2017

axic added in progress and removed in progress labels Jun 13, 2017

pirapira force-pushed the returndata_lll branch from b68347e to 029f9b6 Compare June 13, 2017 14:52

pirapira added a commit that referenced this pull request Jun 13, 2017

Add a failing test as suggested in

2433c60

#2275 (comment)

pirapira and others added 8 commits June 13, 2017 16:57

libevmasm: add RETURNDATACOPY and RETURNDATASIZE

0b22154

Add a changelog entry

5573721

Mention RETURNDATACOPY in GasMeter and SemanticInformation

9ff3064

Warn if returndatasize/returndatacopy is used

05af6c9

test: Add different styles of returndatacopy and returndatasize

464dea2

Add a test about checking a warning

a0f8c94

Add a failing test as suggested in

a7241df

#2275 (comment)

Add a warning about a varialbe of the name of an instruction

8775e77

pirapira force-pushed the returndata_lll branch from 029f9b6 to 8775e77 Compare June 13, 2017 14:57

axic approved these changes Jun 13, 2017

View reviewed changes

chriseth merged commit 6b05224 into develop Jun 13, 2017

chriseth removed the in progress label Jun 13, 2017

pirapira deleted the returndata_lll branch June 13, 2017 18:09

axic mentioned this pull request Jun 30, 2017

Issue warning if an inline assembly instruction shadows a Solidity variable #2308

Closed

Add RETURNDATACOPY and RETURNDATASIZE to assembly (and LLL) #2275

Add RETURNDATACOPY and RETURNDATASIZE to assembly (and LLL) #2275

Conversation

pirapira commented May 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axic left a comment

Choose a reason for hiding this comment

axic commented May 17, 2017 • edited Loading

pirapira commented May 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axic commented May 24, 2017 • edited Loading

axic commented May 24, 2017

chriseth commented May 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axic May 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriseth commented Jun 13, 2017

axic commented Jun 13, 2017

pirapira commented Jun 13, 2017

axic commented May 17, 2017 •

edited

Loading

axic commented May 24, 2017 •

edited

Loading

axic May 24, 2017 •

edited

Loading